Check other potential alises if Docker Hub #605

chanseokoh · 2018-07-13T16:21:55Z

Part of #586.

coollog

Thanks @chanseokoh ! This looks like it works for inferring credential helpers, but #586 would involve adding the alias for the Docker config retrieval (DockerConfigCredentialRetriever)

coollog · 2018-07-13T16:34:43Z

.../src/main/java/com/google/cloud/tools/jib/builder/steps/RetrieveRegistryCredentialsStep.java

+  }
+
+  @VisibleForTesting
+  Authorization findOtherDockerHubCredential(String dockerHubRegistry)


Generally, we want to try to keep the build steps (AsyncStep) as minimal as possible and delegate the real logic to other classes. So, here for example, we should have probably put the credential helper inference logic inside a DockerCredentialHelperInferer rather than in this AsyncStep so that we could unit test against it well.

Just to verify, what I understand from your comments is to minimizing code lines in AsyncStep classes by moving code out to different classes, and not about minimizing operations and time the steps take, right?

chanseokoh · 2018-07-13T18:38:40Z

Looking at the code of RetrieveRegistryCredentialsStep again, looks like it's enough to look inside ~/.docker/config for finding other potential Docker Hub aliases (i.e., no need to change RetrieveRegistryCredentialsStep but only change DockerConfigCredentialRetriever).

I initially thought it would be more complete to do this at the RetrieveRegistryCredentialsStep level so that it would also try other credential supplier, but looks like knownRegistryCredentials is independent of the given registry name and the credential helpers are based only on the given suffix and the commonly known suffixes. @coollog am I getting this right?

chanseokoh · 2018-07-16T19:50:47Z

Ready for review.

I've talked to @coollog, and for now as the first step and a quick fix, this PR just looks into docker.config. However, eventually we should also look for credentials with registry aliases in other sources than docker.config (for example, settings.xml).

chanseokoh · 2018-07-16T19:52:47Z

Ready for review, but honestly

Oops. Was going to say honestly, I am not sure if there should be some special handling for the extra path /v1/ as in index.docker.io/v1/.

briandealwis · 2018-07-16T20:01:32Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAliasGroup.java

+   * @param registry the registry for which the alias group is requested
+   * @return non-empty list of registries where {@code registry} is the first element
+   */
+  public static ImmutableList<String> getAliasesGroup(String registry) {


This method should return List to minimize exposing Guava classes in APIs. Right?

Hmm... I didn't intend for this class to be part of the public API, but I guess there is no way to keep it private to the API while the class is declared public. I'll change it to List.

chanseokoh · 2018-07-16T20:43:22Z

@coollog said the way it manipulated the list to place the given registry at the front (i.e., removing the element and adding it at the front in sequence) did not really feel natural. I agree, so I updated to do it in the functional way.

coollog

Looks good! Just some small comments.

coollog · 2018-07-16T20:00:00Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAliasGroup.java

+import com.google.common.collect.ImmutableList;
+import java.util.ArrayDeque;
+
+public class RegistryAliasGroup {


For consistent style, we include a javadoc for each class.

coollog · 2018-07-16T21:46:53Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAliasGroup.java

+  private static final ImmutableList<ImmutableList<String>> REGISTRY_ALIAS_GROUPS =
+      ImmutableList.of(
+          // Docker Hub alias group
+          ImmutableList.of("registry.hub.docker.com", "index.docker.io"));


This can be a set?

Yeah, no need to be a list.

coollog · 2018-07-16T21:50:22Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAliasGroup.java

+        // Found a group. Move the requested "registry" to the front before returning it.
+        Stream<String> self = Stream.of(registry);
+        Stream<String> withoutSelf = aliasGroup.stream().filter(alias -> !registry.equals(alias));
+        return Stream.concat(self, withoutSelf).collect(Collectors.toList());


Perhaps this could even return the stream itself.

I just tried returning a stream, but it does not seem concise at the caller side. We can switch to Stream later if it seems fit.

Sounds good. For later, the caller could use something like:

Authorization authorization = getAliasesGroup() .map(registryAlias -> retrieve(dockerConfigTemplate, registryAlias)) .filter(Objects::nonNull) .findFirst(); if (authorization.isPresent()) { return authorization.get(); }

Yeah, probably even do .findFirst().orElse(null);

coollog · 2018-07-16T21:53:07Z

jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryAliasGroupTest.java

+
+  @Test
+  public void testGetAliasesGroup_registryHubDockerCom() {
+    Assert.assertArrayEquals(


nit: I think in this codebase, we prefer to use List over raw arrays, such that this can be like Assert.assertEquals(theList, Arrays.asList(a, b, c).

That is certainly better.

coollog · 2018-07-16T21:57:58Z

jib-maven-plugin/CHANGELOG.md

@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.

 - `jib:buildTar` goal to build an image tarball at `target/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514))

+- For Docker Hub, also tries registry aliases when getting a credential from `docker.config`


nit: probably just call it "the Docker config" - docker.config may imply something else
Also, for formatting, it's good to include the pull request/issue number/link and I think this line should probably be under Fixed

+1, also adding this to jib-gradle-plugin.

coollog · 2018-07-16T21:58:51Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAliasGroup.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class RegistryAliasGroup {


For consistent style, we include a javadoc for each class

coollog · 2018-07-17T15:11:14Z

jib-gradle-plugin/CHANGELOG.md

@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.

 - `jibBuildTar` task to build an image tarball at `build/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514))

+- For Docker Hub, also tries registry aliases when getting a credential from the Docker config


small nit: for consistency, we should keep the entries with no empty lines inbetween (line 8 should not actually be there either)

chanseokoh requested a review from a team July 13, 2018 16:21

TadCordle added the PR: Not Ready label Jul 13, 2018

coollog reviewed Jul 13, 2018

View reviewed changes

coollog added this to the v0.9.7 milestone Jul 13, 2018

Try all aliases for retrieving Docker confg creds

b641be1

chanseokoh force-pushed the i586-docker-aliases branch from 67c985d to b641be1 Compare July 16, 2018 19:43

Merge branch 'master' into i586-docker-aliases

8c8c92f

TadCordle removed the PR: Not Ready label Jul 16, 2018

briandealwis reviewed Jul 16, 2018

View reviewed changes

Process list in natural mental model

7c6f5c4

Update CHANGELOG

52b84ab

coollog reviewed Jul 16, 2018

View reviewed changes

chanseokoh added 4 commits July 17, 2018 10:44

CHANGE in jib-gradle-plugin too

ba1f671

better code in tests

0c79c66

Use Set instead of List

aa383fc

Add Javadocs for public classes

5232fb8

coollog approved these changes Jul 17, 2018

View reviewed changes

chanseokoh added 2 commits July 17, 2018 11:13

Merge remote-tracking branch 'origin/master' into i586-docker-aliases

eb21d81

No empty lines in CHANGELOG files

86207ea

chanseokoh merged commit f3a725b into master Jul 17, 2018

chanseokoh deleted the i586-docker-aliases branch July 17, 2018 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check other potential alises if Docker Hub #605

Check other potential alises if Docker Hub #605

chanseokoh commented Jul 13, 2018 •

edited

Loading

coollog left a comment

coollog Jul 13, 2018

chanseokoh Jul 13, 2018

chanseokoh commented Jul 13, 2018

chanseokoh commented Jul 16, 2018 •

edited

Loading

chanseokoh commented Jul 16, 2018

briandealwis Jul 16, 2018

chanseokoh Jul 16, 2018 •

edited

Loading

chanseokoh commented Jul 16, 2018

coollog left a comment

coollog Jul 16, 2018

coollog Jul 16, 2018

chanseokoh Jul 17, 2018

coollog Jul 16, 2018

chanseokoh Jul 17, 2018

coollog Jul 17, 2018

chanseokoh Jul 17, 2018

coollog Jul 16, 2018

chanseokoh Jul 17, 2018

coollog Jul 16, 2018

chanseokoh Jul 17, 2018

coollog Jul 16, 2018

coollog Jul 17, 2018

		@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.

		- `jib:buildTar` goal to build an image tarball at `target/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514))

		- For Docker Hub, also tries registry aliases when getting a credential from `docker.config`

		@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.

		- `jibBuildTar` task to build an image tarball at `build/jib-image.tar`, which can be loaded into docker using `docker load` ([#514](https://github.com/GoogleContainerTools/jib/issues/514))

		- For Docker Hub, also tries registry aliases when getting a credential from the Docker config

Check other potential alises if Docker Hub #605

Check other potential alises if Docker Hub #605

Conversation

chanseokoh commented Jul 13, 2018 • edited Loading

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Jul 13, 2018

chanseokoh commented Jul 16, 2018 • edited Loading

chanseokoh commented Jul 16, 2018

Choose a reason for hiding this comment

chanseokoh Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

chanseokoh commented Jul 16, 2018

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Jul 13, 2018 •

edited

Loading

chanseokoh commented Jul 16, 2018 •

edited

Loading

chanseokoh Jul 16, 2018 •

edited

Loading